[2/n] add newtype wrappers to ensure config identifier validity #69
Conversation
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
sunshowers
changed the base branch from
main
to
sunshowers/spr/main.add-a-newtype-wrapper-to-ensure-config-identifier-validity
sunshowers
changed the title
Created using spr 1.3.6-beta.1 [skip ci]
src/config/identifier.rs
Outdated
| use serde::{Deserialize, Serialize}; | ||
| use thiserror::Error; | ||
|
|
||
| /// A unique identifier for a configuration parameter. |
There was a problem hiding this comment.
Makes sense to codify this a little better, thanks!
| } | ||
| } | ||
|
|
||
| impl<'de> Deserialize<'de> for ConfigIdent { |
There was a problem hiding this comment.
Why manually implement Deserialize but not Serialize?
Is it worth adding a test that we can serialize -> deserialize -> re-serialize?
There was a problem hiding this comment.
Deserialize must go through validation, while serialization doesn't have to. Definitely worth writing a test for.
There was a problem hiding this comment.
Added tests and a comment explaining this.
| @@ -12,10 +12,12 @@ use std::path::Path; | |||
| use thiserror::Error; | |||
| use topological_sort::TopologicalSort; | |||
|
|
|||
| use super::ConfigIdent; | |||
|
|
|||
| /// Describes a set of packages to act upon. | |||
| /// | |||
| /// This structure maps "package name" to "package" | |||
There was a problem hiding this comment.
Do you think it's worth it to make a thin wrapper around ConfigIdent to label this as a PackageName specifically?
I'm assuming that with other configuration parameter usage, there will be other uses of ConfigIdent that are semantically different.
(If you want to punt this beyond this PR, I'm also fine with that)
There was a problem hiding this comment.
Per discussion in DMs, ended up creating three newtypes -- one each for package names, service names and preset names.
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
sunshowers
changed the base branch from
sunshowers/spr/main.add-a-newtype-wrapper-to-ensure-config-identifier-validity
to
main
Created using spr 1.3.6-beta.1
sunshowers
changed the title
sunshowers
deleted the
sunshowers/spr/add-a-newtype-wrapper-to-ensure-config-identifier-validity
branch
I'm going to add some more identifiers soon, and I wanted to make sure at
deserialize time that they were somewhat well-formed. We restrict ourselves to
ASCII at the moment, which is fine.
Add three newtype wrappers -- one each for package, service and preset names.
Depends on: